Provide separate option for std debug asserts #72146
Conversation
|
@Mark-Simulacrum: no appropriate reviewer found, use r? to override |
rust-highfive
added
the
S-waiting-on-review
|
Happy to let @nnethercote approve (or perhaps @alexcrichton has some time, not sure). |
|
@bors: r+ |
|
📌 Commit 6c41545 has been approved by |
bors
added
S-waiting-on-bors
|
Should std debug assertions be enabled at Line 79 in 9682f0e |
|
This shouldn't change behavior of any existing configurations, just allows for more control. So I think no. |
|
I don't have time right now to look into it, but I think that the answer is probably yes -- or at least we should consider using something even more stringent than just debug asserts. I'd be down for example to add a separate Interestingly, looking at a perf profile of a debug asserting std in the rustc libcore case I used for this PR's measurements, I see that a lot of time is spent in core::intrinsics::is_nonoverlapping. It looks like it has 3 separate panic calls in the assembly -- presumably from subtractions and the checked_mul unwrap? That makes its code size pretty large for what it does. I think we should be replacing any panics in the debug asserts in std with just aborts -- it doesn't make sense to pay the cost of unwinding and the panic message being formatted etc. At the very least I stop seeing most of the calls to is_nonoverlapping with this patch applied; presumably they get inlined? It doesn't seem to result in a significant difference in timing though, I'm still at approximately 20 seconds, so maybe this is the wrong approach :/ diff --git a/src/libcore/intrinsics.rs b/src/libcore/intrinsics.rs
index 962bcbe6198..4c8650168c1 100644
--- a/src/libcore/intrinsics.rs
+++ b/src/libcore/intrinsics.rs
@@ -1954,8 +1954,13 @@ pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
pub(crate) fn is_nonoverlapping<T>(src: *const T, dst: *const T, count: usize) -> bool {
let src_usize = src as usize;
let dst_usize = dst as usize;
- let size = mem::size_of::<T>().checked_mul(count).unwrap();
- let diff = if src_usize > dst_usize { src_usize - dst_usize } else { dst_usize - src_usize };
+ let size = mem::size_of::<T>().checked_mul(count).unwrap_or_else(|| unsafe { abort() });
+ // Wrapping subs here are fine because we've already checked the condition
+ let diff = if src_usize > dst_usize {
+ src_usize.wrapping_sub(dst_usize)
+ } else {
+ dst_usize.wrapping_sub(src_usize)
+ };
// If the absolute distance between the ptrs is at least as big as the size of the buffer,
// they do not overlap.
diff >= sizeresults in this assembly: core::intrinsics::is_nonoverlapping /home/mark/Build/rust/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-a1a55821ab603792.so [Percent: local period]
11.47 │ mov %rdx,%rax ▒
12.08 │ mov $0x8,%ecx ▒
9.55 │ mul %rcx ▒
13.37 │ ↓ jo 24 ▒
7.63 │ mov %rdi,%rcx ▒
│ sub %rsi,%rcx ▒
12.11 │ neg %rcx ▒
│ sub %rsi,%rdi ▒
1.92 │ cmovbe %rcx,%rdi ▒
12.74 │ cmp %rax,%rdi ▒
10.23 │ setae %al ▒
8.91 │ ← retq ▒
│24: ud2 ◆
│ ud2 ▒
▒versus the old assembly: core::intrinsics::is_nonoverlapping /home/mark/Build/rust/build/x86_64-unknown-linux-gnu/stage1/lib/librustc_driver-a1a55821ab603792.so [Percent: local period]
9.14 │ push %rax ◆
13.83 │ mov %rdx,%rax ▒
0.93 │ mov $0x8,%ecx ▒
1.88 │ mul %rcx ▒
1.88 │ ↓ jo 43 ▒
1.17 │ mov %rdi,%rcx ▒
8.18 │ sub %rsi,%rcx ▒
│ ↓ jbe 33 ▒
0.94 │ ↓ jae 3b ▒
│ lea str.0,%rdi ▒
│ lea anon.602fa13ea877b28742aac08a06b3d28a.220.llvm.2151656835915294274+0x31d0,%rdx ▒
│ mov $0x21,%esi ▒
│ → callq *0x25f0617(%rip) # 8567168 <core::panicking::panic@Base> ▒
│ ud2 ▒
1.88 │33: sub %rdi,%rsi ▒
1.18 │ mov %rsi,%rcx ▒
0.94 │ ↓ jb 5e ▒
1.88 │3b: cmp %rax,%rcx ▒
0.47 │ setae %al ▒
3.05 │ pop %rcx ▒
52.64 │ ← retq ▒
│43: lea str.0+0x21,%rdi ▒
│ lea anon.602fa13ea877b28742aac08a06b3d28a.220.llvm.2151656835915294274+0x31b8,%rdx ▒
│ mov $0x2b,%esi ▒
│ → callq *0x25f05ec(%rip) # 8567168 <core::panicking::panic@Base> ▒
│ ud2 │5e: lea str.0,%rdi ▒
│ lea anon.602fa13ea877b28742aac08a06b3d28a.220.llvm.2151656835915294274+0x31e8,%rdx ▒
│ mov $0x21,%esi ▒
│ → callq *0x25f05d1(%rip) # 8567168 <core::panicking::panic@Base> ▒
│ ud2 ▒ |
|
Interesting. Aborts don't generate backtraces, which makes them much harder to debug, but that might still be worth it. |
|
Yeah, but we should only be using them in places where unsafe code would otherwise have UB and I think the tradeoff may be worth it. Ideally we'd replace |
Rollup of 7 pull requests Successful merges: - rust-lang#71809 (Use `LocalDefId` in `DumpVisitor::nest_tables`) - rust-lang#72062 (Add built in PSP target) - rust-lang#72146 (Provide separate option for std debug asserts) - rust-lang#72172 (Forbid stage arguments to check) - rust-lang#72173 (Make intra links work inside trait impl block) - rust-lang#72200 (Add prioritize_on attribute to triagebot) - rust-lang#72214 (Minor fixes to comments) Failed merges: r? @ghost
On local one-off benchmarking of libcore metadata-only, debug asserts in std are a significant hit (15s to 20s). Provide an option for compiler developers to disable them. A build with a nightly compiler is around 10s, for reference.